feat: add allocation time unit to instrument picker question - #1681
feat: add allocation time unit to instrument picker question#1681joeshannon wants to merge 1 commit into
Conversation
eee6cd4 to
7600f40
Compare
7600f40 to
89ce084
Compare
| ); | ||
| const call = await callDataSource.getCall(callId); | ||
| if (!call) { | ||
| throw new Error('Call not found'); |
There was a problem hiding this comment.
I see this is in a try block, but if the try fails then it just errors out. Maybe we'd rather go back to fallBackConfig, if we can't get the call? or still show the instrument picker on screen, just without a allocatedTimeUnit?
|
I am a bit confused why we actually need to store the allocationTimeUnit as a config with the question type. As one question could be used for multiple different calls, so could have multiple different allocationTimeUnits that we need to show to users. Surely it would make more sense to generate the allocationTimeUnit that is relevant to the user purely in the frontend by calling a graphQL query and fill in the string at that point in time. Then you wouldn't need to edit the config and put the value in there. What we are risking by putting the allocationTimeUnit in the database here, is that the value in the database is wrong. So we need to verify that it is correct at runtime when the user loads the screen. At that point we wouldn't need it in the database anyway |
Note that modifications made by transformConfig do not get stored in the database: /**
* Function to transform the config value before sending back to the front end.
* Ideal for changes in config at run time.
*/
readonly transformConfig?: (
config: QuestionDataTypeConfigMapping<T>,
callId?: number
) => Promise<QuestionDataTypeConfigMapping<T>>;I've used the same approach as is used for the instruments in the picker where the available instruments are dynamically set based on the call. I agree with your comment in general, although I'm not sure what an alternative solution would look like. The complication arises due to the fact that a |
Description
Motivation and Context
How Has This Been Tested
Fixes
This fixes UserOfficeProject/issue-tracker#1629
Changes
Before:

After:

Depends on
Tests included/Docs Updated?